-
-
Notifications
You must be signed in to change notification settings - Fork 0
Peppy Poppies #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Peppy Poppies #14
Conversation
Added frontend/ and backend/ Added dependency group for frontend and backend
Add basic setup to include directory to separate frontend and backend
Changed logic to accept images, relied on the library pillow for the heavylifting
fixed without any removed code now
should be fine now
Co-authored-by: i-am-unknown-81514525 <74453352+i-am-unknown-81514525@users.noreply.github.com>
Change Size of iframe
janine9vn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Overall Picture
Very nicely done on the project. Despite having a few moving parts, your efforts to make it easy to run via docker paid off. There are some areas in the code where things could be improved, but there's nothing major.
README & Documentation
Your documentation does a great job of showcasing your project and showing the different parts of it. Explaining the breakdown of the different folders was helpful in reviewing your project. Setting up and running the project via docker was also a breeze, nicely done there. An area where your project could improve on is a little bit more explanation and description on how some can add their own questions and the format the question data set to take. The schema doesn't make it the clearest how things are supposed to go together and the format. You can probably piece it together by looking at the existing data set, but explicit documentation on that would go a long way.
Commits
Commits look mostly good. Some messages could use improvement, but that's a small piece of feedback overall. Some of your commits do a bit more than stated, like "add more error handling for runner.js" also alters some of the question set data. It's not the most atomic, but also not the biggest deal. But in those situations, I think it would be beneficial to list out some of the extra things the commit is doing in the commit body itself.
Code Quality
The code is generally easy to read and of good quality. I dive into specific instances in the comments where I have questions or I think things could be improved, so I would take a look there. Some of the file and folder structures seem a bit odd given what's in them. I have specific comments on it you can look at.
|
|
||
| from key import export_key, generate_key_pair, import_private_key, import_public_key | ||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file specifically testing for?
| self._issuer: str = issuer | ||
| self._priv: bytes = get_pem(private_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure these typehints are the most helpful as they can be reliably inferred.
| data["nbf"] = current.timestamp() # Not before timestamp | ||
| data["exp"] = (current + timedelta(seconds=valid_duration)).timestamp() # Expiration timestamp | ||
| data["aud"] = website # Audience (the website domain) | ||
| data["iss"] = self._issuer # The issue (the CAPTCHA server domain) | ||
| data["iat"] = current.timestamp() # Issue timestamp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason the dict keys are kept to 3 letters? It's not immediately clear why they're seemingly hard limited to a length of 3 and therefore need comments explaining what they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is JWT just uses those 3 characters as standard... I would like to not use 3 characters otherwise (since it's confusing but it's also the only way the library can properly validate it)
| print(parsed) | ||
| query_dict = urllib.parse.parse_qs(parsed) | ||
| print(query_dict) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these leftover print statements?
| print(parsed) | ||
| query_dict = urllib.parse.parse_qs(parsed) | ||
| print(query_dict) | ||
| challenge_id = query_dict.get("challenge_id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the possible options challenge_id can be? Seemingly it's either a list or a string?
It's not clear why if it's not eventually a string, then it raises a value error instead of some custom error.
Additionally, based on the function docstring, it's not clear what makes it a valid string or an invalid string.
| from sqlalchemy.ext.asyncio import AsyncSession | ||
|
|
||
|
|
||
| async def provide_challenge_service( # noqa: D103 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as I had with the backend/lib/ folder, it's not clear why this folder-file structure was used.
| ) | ||
|
|
||
|
|
||
| def question_generator(question_set: QuestionSet, seed: int | None = None) -> GeneratedQuestion: # noqa: C901, PLR0915 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels less like a utility that could potentially be used elsewhere and more like something that is necessary and instrinsic to the challenge controller. I would not expect to find this function here and also take up the majority of this file.
| class Part(Struct): | ||
| """A part in question_set.json.""" | ||
|
|
||
| question: str | ||
| validator: str | ||
| range: Ranges |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping this would have more information on what it takes to build a valid question, specifically with the parts. But this doesn't quite have enough information to know how it all comes together. Some more information either here or in a README somewhere about how to format additional questions would be nice.
This comment can also be applied to some other classes in this file as well.
| try: | ||
| from dotenv import load_dotenv | ||
|
|
||
| load_dotenv(override=True) | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this raise some sort of warning if the .env file isn't able to be loaded? If this is intended to be development only, should there be some sort of flag to read/set?
| # This section contains metadata about your project. | ||
| # Don't forget to change the name, description, and authors to match your project! | ||
| name = "pycj12-peppy-poppies" | ||
| description = "Add your description here" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the description? ;P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh lol, looks like we overlooked that.
|
Thanks for the feedback! Our team will look into it. |
No description provided.